Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use only one button for Set featured image and image preview. #14415

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Mar 13, 2019

Description

Refactors the featured image buttons to avoid a focus loss.

The "Set featured image" and the image preview are both buttons. Actually, they're two MediaUploadCheck components: very similar, only the props and children are different.

By using only one MediaUploadCheck and updating props and children, the reference to the "opener" DOM element is not lost and the Media Modal is able to move focus back properly when it closes.

To test:

  • on master:
  • set a featured image (use the mouse, it doesn't matter)
  • observe that when the media modal closes, the image preview does not have the blue outline focus style: that means it's not focused
  • actually, focus is on the body element
  • on this branch:
  • set a featured image
  • when the media modal closes, the image preview does have the focus style

Fixes #14414

@afercia afercia requested a review from youknowriad March 13, 2019 15:50
@jorgefilipecosta jorgefilipecosta added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Media Anything that impacts the experience of managing media labels Mar 14, 2019
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great on my tests and the changes make sense 👍

title={ postLabel.featured_image || DEFAULT_FEATURE_IMAGE_LABEL }
onSelect={ onUpdateImage }
allowedTypes={ ALLOWED_MEDIA_TYPES }
modalClass={ ! featuredImageId ? 'editor-post-featured-image__media-modal' : 'editor-post-featured-image__media-modal' }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both classes are the same -- I think we can just use modalClass="editor-post-featured-image__media-modal".

@jorgefilipecosta jorgefilipecosta merged commit 9a93e6a into master Mar 14, 2019
@jorgefilipecosta jorgefilipecosta deleted the update/featured-image-refactor-buttons-avoid-focus-loss branch March 14, 2019 18:27
@afercia
Copy link
Contributor Author

afercia commented Mar 14, 2019

Thanks @jorgefilipecosta ! Didn't have time to look into the class thing today, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media Anything that impacts the experience of managing media [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus loss when setting featured image
3 participants